Skip to content

Retype all order elements to xsd:positiveInteger and fix the associated examples#477

Closed
skinkie wants to merge 25 commits intomasterfrom
fix_order_positive_integer
Closed

Retype all order elements to xsd:positiveInteger and fix the associated examples#477
skinkie wants to merge 25 commits intomasterfrom
fix_order_positive_integer

Conversation

@skinkie
Copy link
Contributor

@skinkie skinkie commented Jul 28, 2023

This is a breaking change (with respect to the documentation) which mixes xsd:positiveInteger with xsd:integer.

The main difference is: positiveInteger starts at 1.

Please carefully consider the changes that have a mention on "Relative order". No examples exists with a negative relative order, but the design might allow them?

Fix #475

(apply after the required Id pull request)

skinkie and others added 22 commits July 26, 2023 19:52
@skinkie
More required IdType fixes
4ec8571
@skinkie
Typo
cd4a6f3
@skinkie
Remove required ids from DerivedViews
b034777
@skinkie
VehicleJourneyRunTime has a cardinality of 0:1
6a8e19b
@skinkie
Introducing new restrictions, and TODO's
4cf88c0
@skinkie
Better, but still failing
8066af4
@skinkie
More improvements, including required order properties
d6e3660
@ue71603
test for change of one of the TODO
20e6fd6 
something went massively wrong...
@ue71603
Update netex_ifopt_site_version.xsd
2b466ef
@ue71603
Update netex_ifopt_site_version.xsd
f7997ad
@ue71603
Revert "Update netex_ifopt_site_version.xsd"
2301590 
This reverts commit f7997ad.
@ue71603
Revert "Update netex_ifopt_site_version.xsd"
68338ca 
This reverts commit 2b466ef.
@ue71603
Revert "test for change of one of the TODO"
a72adc6 
This reverts commit 20e6fd6.
@skinkie
Fix id on InterchangeRuleTiming on VDV452 example.
4fe6625
@skinkie
Update FlexibleServiceProperties in NOPTIS example.
8abed5f
@skinkie
Fix DestinationDisplayVariant example
7db58d3
@skinkie
Introduce VehicleTypeZoneRestriction id and version attributes
9b167f1
@skinkie
Fix TopographicProjection required id and version
b3b647c
@skinkie
Add required order attribute to FareTableRow
efd996b
@skinkie
Make sure FareTableColumn has a required order element
32f6518
@skinkie
Introduce version and id attributes for AlternativeNameMake sure Fare…
20c04a6 
…TableColumn has a required order element
@skinkie
Retype all order elements to xsd:positiveInteger and fix the associat…
8211ea2 
…ed examples

This is a breaking change (with respect to the documentation) which mixes xsd:positiveInteger with xsd:integer.

The main difference is: positiveInteger starts at 1.

Please carefully consider the changes that have a mention on "Relative order". No examples exists with a negative relative order, but the design might allow them?

Fix #475
@skinkie skinkie self-assigned this Jul 28, 2023
@skinkie skinkie added bug Technical mistake, inconsistency with the documentation, etc. needs documentation update The NeTEx document needs to be updated labels Jul 28, 2023
@trurlurl
Copy link
Collaborator

trurlurl commented Jul 31, 2023

@skinkie No, I cannot be sure about negative order numbers, I'm not sufficiently familiar with the structures in question.

@ue71603
Copy link
Contributor

ue71603 commented Aug 1, 2023

I think we moved the elevator order into an element. So it means that order is arbitrary. And from N to Q everything can be ordered the same way ;-)

@Aurige
Copy link
Contributor

Aurige commented Sep 7, 2023

I agree that we need to harmonise order's type ...
I don't think that negative if the bigger risk (but you never know, you may face people populating order with a lift level), but I'm concerned about 0: you can be sure that a lot of existing data set are already using order=0 when it is defined as an integer... so this means breaking a lot of currently valid data sets.
Why not harmonising the way around (make all orders Integers) ? there is no stake here, except than being able to sort based on the order (we don't care if it is 0 or negative)

@skinkie
Copy link
Contributor Author

skinkie commented Sep 7, 2023

Show me the breaking data.

@Aurige
Copy link
Contributor

Aurige commented Sep 7, 2023

Show me the breaking data.

The examples you had to correct ?

@Aurige Aurige mentioned this pull request Sep 7, 2023
Closed
@skinkie
Copy link
Contributor Author

skinkie commented Sep 7, 2023

The examples you had to correct ?

The PassengerStopAssignment order should always have started at 1?

@Aurige
Copy link
Contributor

Aurige commented Sep 28, 2023

Will be discussed in meeting #14 (October 4th)

@ue71603
Copy link
Contributor

ue71603 commented Sep 28, 2023

Will be discussed in meeting #14 (October 4th)

Does this mean you are ok with it?

@Aurige
Copy link
Contributor

Aurige commented Sep 28, 2023

Will be discussed in meeting #14 (October 4th)

Does this mean you are ok with it?

My mind is not fully made up about it (not against, but don't like its drawbacks)... and anyway whatever I prefer, we need the group validation to accept the drawbacks

@Joostb61
Copy link

Joostb61 commented Oct 20, 2023

The RelativeOrder element should remain integer.
The EPIAP documentation recommends to use RelativeOrder '0' for the groundfloor. That's not possible if it is a positveinteger. However it is possible to renumber the RelativeOrder, this is not desirable since it means all levels get a new RelativeOrdernumber if a new underground floor is added.

@ue71603
Copy link
Contributor

ue71603 commented Oct 20, 2023

@Joostb61 RelativeLevelOrder is not affected. Only the order attributes are concerned.

@ue71603
Copy link
Contributor

ue71603 commented Nov 21, 2023

@Aurige go or no go from group?

@Aurige
Copy link
Contributor

Aurige commented Nov 21, 2023

The group decision was : The PR is accepted but needs to be separated from the IdType fix on which it is stacked
Note: may generate some backward compatibility issues

@ue71603 ue71603 added this to the netex_1.2 milestone Dec 1, 2023
@ue71603
Copy link
Contributor

ue71603 commented Dec 6, 2023

@skinkie who will remove the IdType part? Should I rebase it?. Otherwise it was accepted by enough reviewers. Or do you want explicitly have acceptance by @Aurige

@skinkie
Copy link
Contributor Author

skinkie commented Dec 6, 2023

If you only go for the changing or order element, I would cherry pick the specific commits.

@ue71603 ue71603 assigned ue71603 and unassigned skinkie Jan 17, 2024
@ue71603
Copy link
Contributor

ue71603 commented Jan 24, 2024

use the last four or so comments. and redo...

@Aurige
Copy link
Contributor

Aurige commented Feb 7, 2024 •
edited
Loading

How to express that order (mandatory) is not useful in some cases ? Make it non mandatory ? use value "1" for all orders ?
Value 1 is the group's preferred choice ... to be described in the document/anotation (only when the order is meaningless)

@ue71603 ue71603 mentioned this pull request Feb 7, 2024
Merged
@ue71603
Copy link
Contributor

ue71603 commented Feb 7, 2024

replaces with #657

@ue71603 ue71603 closed this Feb 7, 2024
@trurlurl
Copy link
Collaborator

trurlurl commented Jun 21, 2024

Removing "documentation update needed" since closed without merging.

@trurlurl trurlurl removed the needs documentation update The NeTEx document needs to be updated label Jun 21, 2024
@skinkie skinkie deleted the fix_order_positive_integer branch November 19, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ue71603 ue71603 ue71603 approved these changes

@trurlurl trurlurl trurlurl approved these changes

@nick-knowles nick-knowles Awaiting requested review from nick-knowles

@Aurige Aurige Awaiting requested review from Aurige

Assignees

@ue71603 ue71603

Labels

bug Technical mistake, inconsistency with the documentation, etc.

Projects

None yet

Milestone

netex_1.2

Development

Successfully merging this pull request may close these issues.

Investigate order attribute xsd:integer vs xsd:positiveInteger

5 participants

@skinkie @trurlurl @ue71603 @Aurige @Joostb61